fix(javascript-sdk): use-prompt-none #329

ryanbas21 · 2023-02-09T21:20:17Z

use the prompt=none query parameter.

JIRA Ticket

https://bugster.forgerock.org/jira/browse/SDKS-2060

Description

prompt none provides a better experience when errors occur in an iframe during /authorize

Type of Change

Please Delete options that are not relevant

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

Definition of Done

Check all that apply

Documentation

Acceptance criteria met
Spell-check run
Peer reviewed
Proofread

ryanbas21 · 2023-02-09T21:23:02Z

packages/javascript-sdk/src/oauth2-client/index.ts

+      const error_description_query_param =
+        new URL(response.url).searchParams.get('error_description') !== undefined;
+
+      if (error_description_query_param) {
+        throw new Error(allowedErrors.PromptError);
+      }


honestly unsure if this was really a great way to handle this. need to look into this more

I don't think this is needed. The parseError method used just a few lines down should already grab the error and throw with the right message.

github-actions · 2023-02-09T21:23:17Z

Your preview environment pr-329-ryanbas21 has been deployed.

Preview environment endpoints are available at:

nx-cloud · 2023-02-09T21:24:10Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit f9e2622. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

✅ Successfully ran 5 targets

Sent with 💌 from NxCloud.

cerebrl

Just a few comments.

packages/javascript-sdk/src/oauth2-client/index.ts

cerebrl · 2023-02-11T00:13:25Z

packages/javascript-sdk/src/oauth2-client/index.ts

  CORSError: 'Cross-origin redirection',
+
+  //Prompt error
+  PromptError: 'User is not authenticated or session cookie not sent to the server',


I think the error parameter may be better than the error_description parameter. Rather than this long sentence, you'd look for this: interaction_required.

Actually, looking at this now, the most minimal code change would be to use both the error and error_description as defined by this parseError method: https://github.com/ForgeRock/forgerock-javascript-sdk/blob/develop/packages/javascript-sdk/src/oauth2-client/index.ts#L293.

cerebrl · 2023-02-11T00:19:27Z

packages/javascript-sdk/src/oauth2-client/index.ts

+      const error_description_query_param =
+        new URL(response.url).searchParams.get('error_description') !== undefined;
+
+      if (error_description_query_param) {
+        throw new Error(allowedErrors.PromptError);
+      }


I don't think this is needed. The parseError method used just a few lines down should already grab the error and throw with the right message.

cerebrl · 2023-02-11T00:19:49Z

packages/javascript-sdk/src/oauth2-client/interfaces.ts

 * Options used when requesting the authorization URL.
 */
-interface GetAuthorizationUrlOptions extends ConfigOptions {
+interface GetAuthorizationUrlOptions extends GetTokensOptions {


Was this intentional?

this was because of the above cahnge of passing login, i dont think its needed now, however its more correct from the type perspective i think.

This doesn't feel right to me as the hierarchy of this is in reverse. The TokenManager class, on which getTokens is found, internally uses the OAuthClient class. So, if one inherited from the other, I would think GetTokensOptions would extend GetAuthorizationUrlOptions, not the other way around, yeah? But, ultimately, I don't think either extend the other. For example, forceRenew and login are not properties of GetAuthorizationUrlOptions as the createAuthorizeUrl doesn't use them.

we resolved this on slack. its right and wrong and i've created a ticket to complete a fix for this.

use the prompt=none query parameter.

cerebrl · 2023-02-15T20:49:32Z

packages/javascript-sdk/src/oauth2-client/interfaces.ts

  state?: string;
  verifier?: string;
  query?: StringDict<string>;
+  prompt?: 'none' | 'redirect' | 'consent';


I think you meant "login", rather than "redirect", yeah? Ref: https://backstage.forgerock.com/docs/am/7/oauth2-guide/oauth2-authorize-endpoint.html

cerebrl · 2023-02-15T21:00:16Z

packages/javascript-sdk/src/oauth2-client/interfaces.ts

 * Options used when requesting the authorization URL.
 */
-interface GetAuthorizationUrlOptions extends ConfigOptions {
+interface GetAuthorizationUrlOptions extends GetTokensOptions {


This doesn't feel right to me as the hierarchy of this is in reverse. The TokenManager class, on which getTokens is found, internally uses the OAuthClient class. So, if one inherited from the other, I would think GetTokensOptions would extend GetAuthorizationUrlOptions, not the other way around, yeah? But, ultimately, I don't think either extend the other. For example, forceRenew and login are not properties of GetAuthorizationUrlOptions as the createAuthorizeUrl doesn't use them.

cerebrl · 2023-02-15T21:34:57Z

packages/javascript-sdk/src/oauth2-client/index.ts

  CORSError: 'Cross-origin redirection',
+
+  // prompt=none errors
+  InteractionNotAllowed: 'The request requires some interaction that is not allowed.',


I'm a bit confused here. Being that the parseError method concatenates the error and error_description, I would think this string would start with interaction_required: : https://github.com/ForgeRock/forgerock-javascript-sdk/blob/develop/packages/javascript-sdk/src/oauth2-client/index.ts#L296. Am I misunderstanding what happens here?

Ah, okay, you're right. Sorry for the confusion.

ryanbas21 · 2023-02-15T22:14:03Z

packages/javascript-sdk/src/token-manager/index.ts

-    const authorizeUrlOptions = { ...options, responseType: ResponseType.Code, state, verifier };
-    const authorizeUrl = await OAuth2Client.createAuthorizeUrl(authorizeUrlOptions);
-
+    const authorizeUrlOptions = {


this got formatted /shrug

ryanbas21 · 2023-02-15T22:14:30Z

packages/javascript-sdk/src/token-manager/index.ts

      // Since `login` is configured for "redirect", store authorize values and redirect
      window.sessionStorage.setItem(clientId as string, JSON.stringify(authorizeUrlOptions));
+
+      const authorizeUrl = await OAuth2Client.createAuthorizeUrl(authorizeUrlOptions);


i moved this from above, not needed but its closer to where its used now

BREAKING CHANGE: commit 48cc494 didn't get picked up for breaking change, unsure why so i'm adding it here. there isn o breaking change in this commit

daveadams56

I see this was merged before I hit 'submit' on my review so apologies for not getting feedback to you before this, however I have a couple of questions around the implementation. Could both be things that I missed, but it would be good to discuss these here or in slack when you get time. Thanks.

daveadams56 · 2023-02-16T11:30:25Z

packages/javascript-sdk/src/oauth2-client/interfaces.ts

  state?: string;
  verifier?: string;
  query?: StringDict<string>;
+  prompt?: 'none' | 'login' | 'consent';


My understanding is that the requirement in the ticket was for none to mean that ?prompt=none is passed in the authorize call, login and would be used for centralized login. Maybe a silly question, but what does consent mean here - what is the consequence of specifying this option?

This is a good point. These three values for the /authorize endpoint are valid, but we are likely unable to support the 'consent' option. Should remove this option as to not confuse the dev?

I think this is misleading if it doesn't actually do anything or isn't compatible in the context of the SDK and should be removed. I'd worry that it might inadvertently trick developers into thinking some action should occur if they specify consent, for example showing a consent collection page.

daveadams56 · 2023-02-16T11:33:07Z

packages/javascript-sdk/src/oauth2-client/index.ts

   */
  public static async getAuthCodeByIframe(options: GetAuthorizationUrlOptions): Promise<string> {
-    const url = await this.createAuthorizeUrl(options);
+    const url = await this.createAuthorizeUrl({ ...options, prompt: 'none' });


Given that we are setting prompt=none as a default, does it make sense to make this something the developer can specify? As I understand it, setting prompt: 'login' overrides this but if nothing is set then it's prompt: 'none'. Maybe I'm overthinking this but what do you think?

Good question. This is how I look at it: since this is within the hidden iframe, I don't believe there is ever a reason to NOT use prompt=none as any prompt, aka UI, would be hidden and therefore unusable.

Yes. Given this and my point about the value consent further up, perhaps we should rethink this. Maybe we should obscure this entirely from the user and always pass prompt=none unless the developer specifies they want to do centralized login using login: 'redirect'? That way we avoid introducing a "thing" that the developer has to think about.

ryanbas21 had a problem deploying to Preview February 9, 2023 21:20 — with GitHub Actions Failure

github-actions bot added sdk test labels Feb 9, 2023

ryanbas21 commented Feb 9, 2023

View reviewed changes

cerebrl reviewed Feb 11, 2023

View reviewed changes

ryanbas21 force-pushed the prompt-none branch from 03dc439 to d1ff4f9 Compare February 13, 2023 20:39

ryanbas21 temporarily deployed to Preview February 13, 2023 20:39 — with GitHub Actions Inactive

ryanbas21 force-pushed the prompt-none branch from d1ff4f9 to 38c386b Compare February 13, 2023 20:47

ryanbas21 temporarily deployed to Preview February 13, 2023 20:47 — with GitHub Actions Inactive

github-actions bot added the root label Feb 13, 2023

fix(javascript-sdk): use-prompt-none

c1f4b49

use the prompt=none query parameter.

ryanbas21 force-pushed the prompt-none branch from 38c386b to cc8c879 Compare February 15, 2023 14:52

ryanbas21 temporarily deployed to Preview February 15, 2023 14:52 — with GitHub Actions Inactive

github-actions bot removed the test label Feb 15, 2023

ryanbas21 force-pushed the prompt-none branch from cc8c879 to 38e35fb Compare February 15, 2023 15:32

ryanbas21 temporarily deployed to Preview February 15, 2023 15:32 — with GitHub Actions Inactive

ryanbas21 marked this pull request as ready for review February 15, 2023 17:41

cerebrl requested changes Feb 15, 2023

View reviewed changes

ryanbas21 force-pushed the prompt-none branch from 38e35fb to 310fdab Compare February 15, 2023 22:10

ryanbas21 temporarily deployed to Preview February 15, 2023 22:10 — with GitHub Actions Inactive

github-actions bot added the samples label Feb 15, 2023

ryanbas21 commented Feb 15, 2023

View reviewed changes

chore(javascript-sdk): pr comments

f9e2622

BREAKING CHANGE: commit 48cc494 didn't get picked up for breaking change, unsure why so i'm adding it here. there isn o breaking change in this commit

ryanbas21 force-pushed the prompt-none branch from 310fdab to f9e2622 Compare February 15, 2023 22:27

ryanbas21 temporarily deployed to Preview February 15, 2023 22:27 — with GitHub Actions Inactive

github-actions bot removed the samples label Feb 15, 2023

cerebrl approved these changes Feb 16, 2023

View reviewed changes

ryanbas21 merged commit 36585ed into develop Feb 16, 2023

daveadams56 reviewed Feb 16, 2023

View reviewed changes

cerebrl deleted the prompt-none branch June 12, 2023 16:16

fix(javascript-sdk): use-prompt-none #329

fix(javascript-sdk): use-prompt-none #329

Uh oh!

Conversation

ryanbas21 commented Feb 9, 2023

JIRA Ticket

Description

Type of Change

How Has This Been Tested?

Definition of Done

Documentation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Feb 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nx-cloud bot commented Feb 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

☁️ Nx Cloud Report

Uh oh!

cerebrl left a comment

Choose a reason for hiding this comment

Uh oh!

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!

daveadams56 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!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

github-actions bot commented Feb 9, 2023 •

edited

Loading

nx-cloud bot commented Feb 9, 2023 •

edited

Loading