Iframe manager #253

ryanbas21 · 2025-04-19T15:15:11Z

JIRA Ticket

https://pingidentity.atlassian.net/browse/SDKS-3904

Description

Initial rewrite of iframe manager

changeset-bot · 2025-04-19T15:15:15Z

🦋 Changeset detected

Latest commit: 04b506c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 8 packages

Name	Type
@forgerock/iframe-manager	Minor
@forgerock/sdk-oidc	Minor
@forgerock/sdk-types	Minor
@forgerock/davinci-client	Minor
@forgerock/storage	Minor
@forgerock/sdk-utilities	Minor
@forgerock/sdk-logger	Minor
@forgerock/sdk-request-middleware	Minor

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

nx-cloud · 2025-04-19T15:16:00Z

View your CI Pipeline Execution ↗ for commit 04b506c.

Command	Status	Duration	Result
`nx affected -t build typecheck lint test e2e-ci`	✅ Succeeded	2m 2s	View ↗
`nx-cloud record -- nx format:check`	✅ Succeeded	3s	View ↗

☁️ Nx Cloud last updated this comment at 2025-05-01 19:35:19 UTC

github-actions · 2025-04-19T15:18:44Z

Deployed 800732c to https://ForgeRock.github.io/ping-javascript-sdk/pr-253/800732cf7d06bf3fa9e29cc7ffbf5d1b63df3c25 branch gh-pages in ForgeRock/ping-javascript-sdk

codecov-commenter · 2025-04-19T15:18:45Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 49.33%. Comparing base (87c200b) to head (04b506c).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #253      +/-   ##
==========================================
- Coverage   49.45%   49.33%   -0.13%     
==========================================
  Files          29       29              
  Lines        1567     1571       +4     
  Branches      172      173       +1     
==========================================
  Hits          775      775              
- Misses        792      796       +4

see 1 file with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

ryanbas21 · 2025-04-19T18:32:23Z

packages/sdk-utilities/src/lib/authorize/authorize.types.ts

@@ -1,10 +1,3 @@
-/*


have to put this back

cerebrl

Preliminary review.

cerebrl · 2025-04-28T20:32:58Z

packages/sdk-effects/iframe-manager/src/lib/iframe-manager.effects.ts

+            if (
+              newHref === 'about:blank' ||
+              !newHref.startsWith(options.url.substring(0, options.url.indexOf('?')))
+            ) {
+              // Check if the newHref origin matches expected redirect_uri origin if possible
+              // Or simply check if it contains expected parameters.
+              // For now, we proceed assuming any load could be the redirect.
+            }


What is the purpose of this if we don't do anything with the condition?

cerebrl · 2025-04-28T20:45:00Z

packages/sdk-effects/iframe-manager/src/lib/iframe-manager.effects.ts

+   * @param options - The options for the iframe request (URL, timeout).
+   * @returns A Promise that resolves with the query parameters from the redirect URL or rejects on timeout or error.
+   */
+  const getAuthCodeByIFrame = (options: {


Is there a way to make this more generic? In other words, could this just be getValuesByIframe? So, this just creates an iframe in a generic way, and detects for the values that are given as arguments. That way, this function doesn't have to know what values to look for or what's relevant. It just manages the iframe, the event listeners, the cleanup, etc. So, it could be useful for getting a Authorization Code or anything else. I'd say, it needs success values and error values to listen for.

ancheetah · 2025-04-29T14:35:33Z

packages/sdk-utilities/src/lib/authorize/authorize.types.ts

+ */
+export type ResponseType = 'code' | 'token';
+
+export interface GetAuthorizationUrlOptions extends LegacyConfigOptions {


Why are authorize types in the utilities package? They don't seem to be used here. Did you mean to place them in the sdk-types or effects package?

They are used below, I can move it to the types package though, I don't have a strong opinion, but we should probably try to create some concrete rules as to when a type goes to types versus living with other types.

Can you point out where below? I'm not seeing these types used in utilities.

https://github.com/ForgeRock/ping-javascript-sdk/pull/253/files#diff-a5ae838afdfc600ea1dae4aed3f3378c41f679d1bfd989e13e1233354689a81dR40

I just meant it's used in this file, but yeah we can move it thats fine by me

ancheetah · 2025-04-29T14:39:46Z

packages/sdk-effects/iframe-manager/src/lib/iframe-manager.effects.ts

+   * @returns A Promise that resolves with the query parameters from the redirect URL or rejects on timeout or error.
+   */
+  const getAuthCodeByIFrame = (options: {
+    url: AuthorizeUrl;


Just wondering why we opted for a url param here instead of creating an authorize url based off some options passed in like the legacy sdk does? With the url param I would think a disadvantage for the end user is knowing how to construct a well formed authorize url.

Justin wanted us to pass in the authorize url here rather than create it in this function

Yes, this is in an effort to generalize this iframe function and "flatten" what these functions do for better composability.

Let's make sure to remove the AuthorizeUrl type dependency here.

cerebrl · 2025-04-30T19:22:26Z

packages/sdk-effects/iframe-manager/src/lib/iframe-manager.effects.ts

+ */
+
+/* eslint-disable @typescript-eslint/no-empty-function */
+import { AuthorizeUrl } from '@forgerock/sdk-types';


Since this no longer has any "Authorize" related functions, we should remove the need for this type.

cerebrl · 2025-04-30T19:24:23Z

packages/sdk-effects/iframe-manager/src/lib/iframe-manager.effects.ts

+ *
+ * @returns An object containing the API for managing iframe requests.
+ */
+export default function iframeManagerInit(/*config: OAuthConfig*/) {


Let's try to keep names as short as possible. Being that all effects are going to follow this pattern of calling a function that returns an API "instance", I don't think there's a need to explicitly have "init" in the name. We can just call it iframeManager or iframeMgmt.

cerebrl · 2025-04-30T19:24:54Z

packages/sdk-effects/iframe-manager/src/lib/iframe-manager.effects.ts

+ *
+ * @returns An object containing the API for managing iframe requests.
+ */
+export default function iframeManagerInit(/*config: OAuthConfig*/) {


Let's also remove the comment /*config: OAuthConfig*/.

cerebrl · 2025-04-30T19:25:22Z

packages/sdk-effects/iframe-manager/src/lib/iframe-manager.effects.ts

+   * @returns A Promise that resolves with the query parameters from the redirect URL or rejects on timeout or error.
+   */
+  const getAuthCodeByIFrame = (options: {
+    url: AuthorizeUrl;


Let's make sure to remove the AuthorizeUrl type dependency here.

cerebrl · 2025-04-30T19:29:24Z

packages/sdk-effects/iframe-manager/src/lib/iframe-manager.effects.ts

+   * @param options - The options for the iframe request (URL, timeout).
+   * @returns A Promise that resolves with the query parameters from the redirect URL or rejects on timeout or error.
+   */
+  const getParamsFromIFrame = (options: {


Not sure I like this name. How about getRedirectParams? The fact that we're using an iframe to do it is inherent in the fact that this is the iframe manager API.

cerebrl · 2025-04-30T19:53:22Z

packages/sdk-effects/iframe-manager/src/lib/iframe-manager.effects.ts

+  return {
+    getParamsFromIFrame,
+  };


I'm not a huge fan of this pattern. Can we keep this consistent with our other modules and return the object with the methods defined inline?

cerebrl · 2025-04-30T20:03:04Z

packages/sdk-effects/iframe-manager/src/lib/iframe-manager.effects.ts

+            });
+
+            // Check for standard OAuth error parameters
+            if (params.error) {


This block is still quite tied to OAuth related stuff. Let's add error params to the config parameter too. In other words, let's have an array of success params, and an array of error params. That way, this function doesn't need to know how to do anything other than the following:

Does this URL have any of the provided success params -> resolve with all params as object

Does this URL have any of the provided error params -> reject with all params as object

cerebrl · 2025-04-30T20:08:51Z

packages/sdk-effects/iframe-manager/src/lib/iframe-manager.effects.ts

+        cleanup = () => {};
+      };
+
+      onLoadHandler = (): void => {


I think the thing we're missing here is waiting for either a success or error param before resolving/rejecting. If I'm reading this right, we are assuming success or failure upon the first redirect. But, this may not always be true. There may be additional redirects on the server-side before the redirect back to the client app.

That's what this was for in the legacy code: https://github.com/ForgeRock/forgerock-javascript-sdk/blob/develop/packages/javascript-sdk/src/oauth2-client/index.ts#L127. So, there's a condition saying, "now that the iframe has loaded a page, does it contain the appropriate success or error param? If not, keep listening for more onload events."

cerebrl · 2025-04-30T20:09:38Z

packages/sdk-effects/iframe-manager/src/lib/iframe-manager.effects.ts

+              // Reject with error details from the URL
+              reject(
+                new Error(
+                  `Authorization error: ${params.error}. Description: ${params.error_description || 'N/A'}`,


More "Authorization" specific stuff that we should genericize.

cerebrl · 2025-05-01T16:20:52Z

packages/sdk-effects/iframe-manager/src/lib/iframe-manager.effects.ts

+            cleanup();
+            reject({
+              type: 'internal_error',
+              message: 'unexpected failure',


We don't have to do it now, but I'd like to think about how we can provide troubleshooting hints in our error messages. This may reduce support tickets for us to give them simple debugging tips, like copy and paste the URL into the browser to see the issue.

create an iframe manager package which is consumed by davinci-client. Updated types throughout the codebase add-headers to files

ryanbas21 commented Apr 19, 2025

View reviewed changes

packages/sdk-utilities/src/lib/authorize/authorize.types.ts Outdated

@@ -1,10 +1,3 @@

/*

Copy link

Collaborator Author

ryanbas21 Apr 19, 2025

Choose a reason for hiding this comment

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

have to put this back

cerebrl requested changes Apr 28, 2025

View reviewed changes

ryanbas21 force-pushed the iframe-manager branch from 7bba5de to 7aaad9d Compare April 28, 2025 20:49

ancheetah reviewed Apr 29, 2025

View reviewed changes

ryanbas21 force-pushed the iframe-manager branch from 7aaad9d to ccdaff9 Compare April 30, 2025 18:06

ryanbas21 marked this pull request as ready for review April 30, 2025 18:37

ryanbas21 force-pushed the iframe-manager branch from c2a5f62 to b398fa8 Compare April 30, 2025 18:44

cerebrl requested changes Apr 30, 2025

View reviewed changes

cerebrl reviewed May 1, 2025

View reviewed changes

cerebrl approved these changes May 1, 2025

View reviewed changes

ryanbas21 force-pushed the iframe-manager branch 6 times, most recently from ed203dc to 1667343 Compare May 1, 2025 19:26

ancheetah approved these changes May 1, 2025

View reviewed changes

feat(iframe-manager): create-iframe-manager

04b506c

create an iframe manager package which is consumed by davinci-client. Updated types throughout the codebase add-headers to files

ryanbas21 force-pushed the iframe-manager branch from 1667343 to 04b506c Compare May 1, 2025 19:32

ryanbas21 merged commit 41f48fe into main May 1, 2025
4 checks passed

ryanbas21 deleted the iframe-manager branch May 1, 2025 19:37

ryanbas21 mentioned this pull request May 1, 2025

Release PR #257

Merged

Iframe manager #253

Iframe manager #253

Uh oh!

Conversation

ryanbas21 commented Apr 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

JIRA Ticket

Description

Uh oh!

changeset-bot bot commented Apr 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

nx-cloud bot commented Apr 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Apr 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Apr 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cerebrl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

ryanbas21 commented Apr 19, 2025 •

edited

Loading

changeset-bot bot commented Apr 19, 2025 •

edited

Loading

nx-cloud bot commented Apr 19, 2025 •

edited

Loading

github-actions bot commented Apr 19, 2025 •

edited

Loading

codecov-commenter commented Apr 19, 2025 •

edited

Loading