feat(react): add SignIn component and create sample app #14
Conversation
… branding from console cannot be fetched
take the complete authenticator object as a parameter
| if ((await AuthClient.getInstance().getDataLayer().getConfigData()).enableConsoleBranding ?? true) { | ||
| brandingFromConsole = await getBrandingPreference(); | ||
| try { | ||
| if ((await AuthClient.getInstance().getDataLayer().getConfigData()).enableConsoleBranding ?? true) { |
There was a problem hiding this comment.
Let's move this to a variable for better readability
There was a problem hiding this comment.
Will do this in the next PR
| brandingFromConsole = await getBrandingPreference(); | ||
| try { | ||
| if ((await AuthClient.getInstance().getDataLayer().getConfigData()).enableConsoleBranding ?? true) { | ||
| brandingFromConsole = await getBrandingPreference(); |
There was a problem hiding this comment.
brandingFromConsole is this term correct? We are not getting branding from the console rather from the IS/Asgardeo
There was a problem hiding this comment.
I agree. Since we don't have a unified name yet, there was a discussion sometime back to use more common words without siding with either of them. Any suggestions for a better name?
|
|
||
| const configData: AuthClientConfig<UIAuthConfig> = await AuthClient.getInstance().getDataLayer().getConfigData(); | ||
|
|
||
| const DEFAULT_NAME: string = 'carbon.super'; |
There was a problem hiding this comment.
Shall we move this to a default value of the AuthConfig itself?
There was a problem hiding this comment.
Will address this in the coming PR.
Is the actual default value carbon.super?
| authenticate, | ||
| authorize, | ||
| getBranding, | ||
| keys, |
There was a problem hiding this comment.
| keys, | |
| keys | |
| `` | |
| It looks like linter is not working properly. Check and see. It should catch these formatting errors |
| authorize() | ||
| .then((response: AuthApiResponse) => { | ||
| setAuthResponse(response); | ||
| setIsComponentLoading(false); | ||
| }) | ||
| .catch((error: Error) => { | ||
| setAlert({alertType: {error: true}, key: keys.common.error}); | ||
| setIsComponentLoading(false); | ||
| throw new AsgardeoUIException('REACT_UI-SIGN_IN-SI-SE01', 'Authorization failed', error.stack); | ||
| }); |
There was a problem hiding this comment.
Does it make sense to call this inside this hook? This will cause the authorize call to fire when the dependency array changes
There was a problem hiding this comment.
Yes, I will move this out.
| getBranding({branding: brandingProps, merged: brandingPreference}).then((response: Branding) => { | ||
| setComponentBranding(response); | ||
| }); |
There was a problem hiding this comment.
We might be able to convert this to a hook like useBranding()
| setAlert(undefined); | ||
|
|
||
| if (authResponse === undefined) { | ||
| throw new AsgardeoUIException('REACT_UI-SIGN_IN-HA-IV02', 'Auth response is undefined.'); |
There was a problem hiding this comment.
I hope you have documented these error codes and their meaning somewhere
There was a problem hiding this comment.
I'm following the same format in Asgardeo Auth-JS. This is the format.
will add this to the docs and readme later.
| const {code, state} = event.data; | ||
|
|
||
| if (code && state) { | ||
| handleAuthenticate(resp.nextStep.authenticators[0].authenticatorId, {code, state}); |
There was a problem hiding this comment.
| handleAuthenticate(resp.nextStep.authenticators[0].authenticatorId, {code, state}); | |
| handleAuthenticate(resp?.nextStep?.authenticators[0]?.authenticatorId, {code, state}); |
Add null checks. Check in other places as well
| return ( | ||
| <UISignIn.Paper className="multiple-options-paper"> | ||
| <UISignIn.Typography title className="multiple-otions-title"> | ||
| Sign In |
There was a problem hiding this comment.
Shouldn't we access this from text customization
There was a problem hiding this comment.
This is kind of a special scenario where multiple authenticators are there without username & password. So this text doesn't belong to a specific authenticator screen. Could add this to the common set of texts. But doesn't seem much logical to add it there as well. WDYT?
| {authResponse?.flowStatus !== FlowStatus.SuccessCompleted && !isAuthenticated && ( | ||
| <> | ||
| {renderSignIn()} | ||
|
|
There was a problem hiding this comment.
Wouldn't these spacing be better for readability?
| </UISignIn.Link> | ||
| ), | ||
| }, | ||
| {children: <UISignIn.Typography>{componentBranding?.locale ?? 'en-US'}</UISignIn.Typography>}, |
There was a problem hiding this comment.
Let's keep the fallback locale in a constant
There was a problem hiding this comment.
Will address this in the next PR.
| showSelfSignUp, | ||
| renderLoginOptions, | ||
| }: BasicAuthProps): JSX.Element => { | ||
| const [username, setUsername] = useState(''); |
There was a problem hiding this comment.
| const [username, setUsername] = useState(''); | |
| const [username, setUsername] = useState<string>(''); |
Define types for states. Check in other places as well
| if (isLoading) { | ||
| return ( | ||
| <div className="circular-progress-holder"> | ||
| <CircularProgress className="circular-progress" /> | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
| if (isLoading) { | |
| return ( | |
| <div className="circular-progress-holder"> | |
| <CircularProgress className="circular-progress" /> | |
| </div> | |
| ); | |
| } | |
| if (isLoading) => ( | |
| <div className="circular-progress-holder"> | |
| <CircularProgress className="circular-progress" /> | |
| </div> | |
| ); |
There was a problem hiding this comment.
I don't think that if statements support arrow functions like this.
| window.location.reload(); | ||
| signOutApiCall().then(() => { | ||
| sessionStorage.clear(); | ||
| window.location.reload(); |
There was a problem hiding this comment.
Remove this logic and expose an onSignOut hooks so the app user can handle it themselves
| alertType: | ||
| | {error?: boolean; info?: never; warning?: never} | ||
| | {error?: never; info?: boolean; warning?: never} | ||
| | {error?: never; infor?: never; warning?: boolean}; |
There was a problem hiding this comment.
Why can't we use an enum here?
| dangerouslySetInnerHTML={{ | ||
| __html: JSON.stringify(user, null, 2).replace( | ||
| /\n/g, | ||
| "<br />" |
There was a problem hiding this comment.
Avoid using dangerouslySetInnerHTML as this could lead to XSS attacks
| const config: UIAuthConfig = { | ||
| baseUrl: "https://localhost:9443", | ||
| clientID: "b1uRjwpqydvxjGR42Y6BnIdQMRMa", | ||
| scope: ["openid", "internal_login", "profile"], | ||
| signInRedirectURL: "https://localhost:5173", | ||
| enableConsoleTextBranding: true, | ||
| }; | ||
|
|
||
| const devConfig: UIAuthConfig = { | ||
| baseUrl: "https://dev.api.asgardeo.io/t/movinorg", | ||
| clientID: "kH5OfXOvpGLOvp1iAw4zQmNvv4oa", | ||
| scope: ["openid", "internal_login", "profile"], | ||
| signInRedirectURL: "https://localhost:5173", | ||
| }; |
There was a problem hiding this comment.
Don't hardcode this within the code. Let's read from a config.json file or a .env file
3 participants
Purpose
Related Issues
Related PRs
Checklist
Security checks