Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Feb 16, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c1f4b49
fix(javascript-sdk): use-prompt-none
ryanbas21 Feb 9, 2023
f9e2622
chore(javascript-sdk): pr comments
ryanbas21 Feb 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 6 additions & 2 deletions packages/javascript-sdk/src/oauth2-client/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@ const allowedErrors = {
NetworkError: 'NetworkError when attempting to fetch resource.',
// Webkit browser error
CORSError: 'Cross-origin redirection',

// prompt=none errors
InteractionNotAllowed: 'The request requires some interaction that is not allowed.',
Copy link
Contributor

@cerebrl cerebrl Feb 15, 2023

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@ryanbas21 ryanbas21 Feb 15, 2023

Choose a reason for hiding this comment

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

Screen Shot 2023-02-15 at 2 52 49 PM

Copy link
Contributor

@cerebrl cerebrl Feb 15, 2023

Choose a reason for hiding this comment

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

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

};

/**
Expand All @@ -46,14 +49,14 @@ const allowedErrors = {
abstract class OAuth2Client {
public static async createAuthorizeUrl(options: GetAuthorizationUrlOptions): Promise<string> {
const { clientId, middleware, redirectUri, scope } = Config.get(options);

const requestParams: StringDict<string | undefined> = {
...options.query,
client_id: clientId,
redirect_uri: redirectUri,
response_type: options.responseType,
scope,
state: options.state,
...(options.prompt ? { prompt: options.prompt } : {}),
};

if (options.verifier) {
Expand Down Expand Up @@ -82,7 +85,8 @@ abstract class OAuth2Client {
* New Name: getAuthCodeByIframe
*/
public static async getAuthCodeByIframe(options: GetAuthorizationUrlOptions): Promise<string> {
const url = await this.createAuthorizeUrl(options);
const url = await this.createAuthorizeUrl({ ...options, prompt: 'none' });
Copy link
Contributor

@daveadams56 daveadams56 Feb 16, 2023

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@cerebrl cerebrl Feb 16, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@daveadams56 daveadams56 Feb 16, 2023

Choose a reason for hiding this comment

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

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.


const { serverConfig } = Config.get(options);

return new Promise((resolve, reject) => {
Expand Down
1 change: 1 addition & 0 deletions packages/javascript-sdk/src/oauth2-client/interfaces.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ interface GetAuthorizationUrlOptions extends ConfigOptions {
state?: string;
verifier?: string;
query?: StringDict<string>;
prompt?: 'none' | 'login' | 'consent';
Copy link
Contributor

@daveadams56 daveadams56 Feb 16, 2023

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@cerebrl cerebrl Feb 16, 2023

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@daveadams56 daveadams56 Feb 16, 2023

Choose a reason for hiding this comment

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

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.

}

/**
Expand Down
13 changes: 10 additions & 3 deletions packages/javascript-sdk/src/token-manager/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,9 +118,12 @@ abstract class TokenManager {
*/
const verifier = PKCE.createVerifier();
const state = PKCE.createState();
const authorizeUrlOptions = { ...options, responseType: ResponseType.Code, state, verifier };
const authorizeUrl = await OAuth2Client.createAuthorizeUrl(authorizeUrlOptions);

const authorizeUrlOptions = {
Copy link
Contributor Author

@ryanbas21 ryanbas21 Feb 15, 2023

Choose a reason for hiding this comment

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

this got formatted /shrug

...options,
responseType: ResponseType.Code,
state,
verifier,
};
/**
* Attempt to call the authorize URL to retrieve authorization code
*/
Expand Down Expand Up @@ -155,6 +158,7 @@ abstract class TokenManager {
allowedErrors.AuthorizationTimeout !== err.message &&
allowedErrors.FailedToFetch !== err.message &&
allowedErrors.NetworkError !== err.message &&
allowedErrors.InteractionNotAllowed !== err.message &&
// Safari has a very long error message, so we check for a substring
!err.message.includes(allowedErrors.CORSError)
) {
Expand All @@ -165,6 +169,9 @@ abstract class TokenManager {

// 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);
Copy link
Contributor Author

@ryanbas21 ryanbas21 Feb 15, 2023

Choose a reason for hiding this comment

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

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


return window.location.assign(authorizeUrl);
}

Expand Down